-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add video widget admin side component and resource #1828
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1828 +/- ##
==========================================
- Coverage 71.47% 70.89% -0.58%
==========================================
Files 396 405 +9
Lines 12051 12327 +276
Branches 910 923 +13
==========================================
+ Hits 8613 8739 +126
- Misses 3291 3431 +140
- Partials 147 157 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
sa.Column('description', sa.String)) | ||
|
||
op.bulk_insert(widget_type_table, [ | ||
{'id': 7, 'name': 'Video', 'description': 'Add a link to a hosted video and link preview'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in one of the alembic scripts , i haev a better of handling the increment numbers.. can you look up that?
basically it reads value from the sequnce and does a plus one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely a smarter way to do it. I just choose to hard code 7 here because it is hard coded in the front end:
And types are hard coded in the backend in the constants in WidgetType which is used for the tests. I wasn't comfortable leaving it up for a calculation although it will reach the same result of 7. What do you think
}, | ||
}); | ||
const response = await http.PatchRequest<VideoWidget>(url, data); | ||
if (response.data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return response.data || Promise.reject(' etc
works? the if statement looks like long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will fix in the next PR directly after this
if (response.data) { | ||
return response.data; | ||
} | ||
return Promise.reject('Failed to create video widget'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same like below..may be use a || to avoid if
make sure it works please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will fix in the next PR directly after this
* Add video widget model and option card * add video widget service and resource * Add video widget crud and UI component * Fix linting issue
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).